Skip to content

feat: unify prerequisite validation via IRequirementCheck + IPrerequisiteRunner (#106)#312

Merged
sellakumaran merged 12 commits intomainfrom
users/sellak/failEarly
Mar 10, 2026
Merged

feat: unify prerequisite validation via IRequirementCheck + IPrerequisiteRunner (#106)#312
sellakumaran merged 12 commits intomainfrom
users/sellak/failEarly

Conversation

@sellakumaran
Copy link
Copy Markdown
Contributor

Commands now declare prerequisites using IRequirementCheck and fail early with actionable messages before any side effects occur.

Phase 1 - pure reorganization (zero behavioral change):

  • Add AzureAuthRequirementCheck and InfrastructureRequirementCheck adapters
  • Add IPrerequisiteRunner / PrerequisiteRunner to run checks in order
  • Route AllSubcommand, BlueprintSubcommand, InfrastructureSubcommand, and DeployCommand through the shared runner instead of ad-hoc validators
  • Delete dead code: ISubCommand.ValidateAsync, IAzureValidator/AzureValidator
  • Make AzureAuthValidator.ValidateAuthenticationAsync virtual for testability

Phase 2 - minimal early-fail additions:

  • cleanup azure: auth check before preview display
  • deploy mcp: explicit early guards for agentBlueprintId and agenticAppId before any Graph/network calls

Closes #106

…siteRunner (#106)

Commands now declare prerequisites using IRequirementCheck and fail early
with actionable messages before any side effects occur.

Phase 1 - pure reorganization (zero behavioral change):
- Add AzureAuthRequirementCheck and InfrastructureRequirementCheck adapters
- Add IPrerequisiteRunner / PrerequisiteRunner to run checks in order
- Route AllSubcommand, BlueprintSubcommand, InfrastructureSubcommand,
  and DeployCommand through the shared runner instead of ad-hoc validators
- Delete dead code: ISubCommand.ValidateAsync, IAzureValidator/AzureValidator
- Make AzureAuthValidator.ValidateAuthenticationAsync virtual for testability

Phase 2 - minimal early-fail additions:
- cleanup azure: auth check before preview display
- deploy mcp: explicit early guards for agentBlueprintId and agenticAppId
  before any Graph/network calls

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@sellakumaran sellakumaran requested review from a team as code owners March 6, 2026 23:36
Copilot AI review requested due to automatic review settings March 6, 2026 23:36
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 6, 2026

⚠️ Deprecation Warning: The deny-licenses option is deprecated for possible removal in the next major release. For more information, see issue 997.

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR standardizes CLI prerequisite validation by introducing a shared IPrerequisiteRunner that executes IRequirementCheck implementations, replacing ad-hoc validators and enabling consistent early-fail behavior across commands.

Changes:

  • Added IPrerequisiteRunner/PrerequisiteRunner plus new requirement checks (AzureAuthRequirementCheck, InfrastructureRequirementCheck) and corresponding unit tests.
  • Routed setup and deploy flows through the shared runner; removed legacy IAzureValidator/AzureValidator and ISubCommand.ValidateAsync.
  • Added additional early guards for deploy mcp to fail before Graph/network calls when required IDs are missing.

Reviewed changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
src/Microsoft.Agents.A365.DevTools.Cli/Services/Requirements/RequirementChecks/InfrastructureRequirementCheck.cs Adds config-only infrastructure field validation as a reusable requirement check.
src/Microsoft.Agents.A365.DevTools.Cli/Services/Requirements/RequirementChecks/AzureAuthRequirementCheck.cs Adds an adapter requirement check over AzureAuthValidator.
src/Microsoft.Agents.A365.DevTools.Cli/Services/IPrerequisiteRunner.cs Introduces interface for running prerequisite checks.
src/Microsoft.Agents.A365.DevTools.Cli/Services/PrerequisiteRunner.cs Implements centralized prerequisite execution and logging.
src/Microsoft.Agents.A365.DevTools.Cli/Services/AzureAuthValidator.cs Makes ValidateAuthenticationAsync virtual for testability.
src/Microsoft.Agents.A365.DevTools.Cli/Program.cs Rewires DI: removes IAzureValidator, registers IPrerequisiteRunner, passes new deps to commands.
src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupCommand.cs Updates setup command wiring to use prerequisite runner + auth/environment validators.
src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AllSubcommand.cs Replaces manual validation collection with requirement checks + runner; invokes environment validator.
src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/InfrastructureSubcommand.cs Uses prerequisite runner for Azure auth + infra config checks before provisioning.
src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs Uses prerequisite runner for Azure auth prior to blueprint creation.
src/Microsoft.Agents.A365.DevTools.Cli/Commands/DeployCommand.cs Uses prerequisite runner for Azure auth in deploy flows; adds early guards in deploy mcp.
src/Microsoft.Agents.A365.DevTools.Cli/Commands/CleanupCommand.cs Adds prerequisite runner + auth validator to Azure cleanup subcommand.
src/Microsoft.Agents.A365.DevTools.Cli/Commands/CreateInstanceCommand.cs Removes Azure validation dependency from deprecated command (still exits immediately).
src/Microsoft.Agents.A365.DevTools.Cli/Services/AzureValidator.cs Removes legacy unified Azure validator.
src/Microsoft.Agents.A365.DevTools.Cli/Services/ISubCommand.cs Removes legacy subcommand validation interface.
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Requirements/InfrastructureRequirementCheckTests.cs Adds unit tests for InfrastructureRequirementCheck.
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Requirements/AzureAuthRequirementCheckTests.cs Adds unit tests for AzureAuthRequirementCheck.
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/PrerequisiteRunnerTests.cs Adds unit tests for PrerequisiteRunner behavior (pass/fail/warn/order).
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/SubcommandValidationTests.cs Updates infra validation tests to use requirement checks instead of removed validators.
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/SetupCommandTests.cs Updates command construction/tests for new prerequisite runner + validators.
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/DeployCommandTests.cs Updates deploy command tests for new prerequisite runner + auth validator injection.
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/CreateInstanceCommandTests.cs Updates test wiring due to removed Azure validator parameter.
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/CleanupCommandTests.cs Updates cleanup tests for new prerequisite runner + auth validator injection.
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/CleanupCommandBotEndpointTests.cs Updates bot-endpoint cleanup tests for new prerequisite runner + auth validator injection.
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/BlueprintSubcommandTests.cs Updates blueprint subcommand tests to use prerequisite runner + auth validator instead of removed Azure validator.

You can also share your feedback on Copilot code review. Take the survey.

sellakumaran and others added 2 commits March 6, 2026 16:08
- ConfigFileNotFoundException now extends Agent365Exception so missing
  config errors surface as clean user messages (no stack trace) on all
  commands, not just those with local catch blocks. Removes ad-hoc
  FileNotFoundException catches in CleanupCommand and CreateInstanceCommand.

- config init: expand relative/dot deployment paths to absolute before
  saving so the stored value is portable across directories. Update help
  text to clarify relative paths are accepted.

- config init: drop platform-specific parenthetical from 'Allow public
  client flows' log message -- the setting is required on all platforms.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Move "Running all setup steps..." to after requirements check output
- Remove redundant "Agent 365 Setup" header (user already knows the command)
- Change CorrelationId log to LogDebug for setup all and blueprint; surface
  as TraceId inline on the action line ("Running all setup steps... (TraceId: ...)")
  so it is always captured in setup.log as [INF] and visible on console
- Demote PlatformDetector internal logs to LogDebug; single "Detected project
  platform: X" line remains as the user-facing output
- Add AzureAuthRequirementCheck to GetConfigRequirementChecks so Azure auth
  appears in requirements output for all setup subcommands
- Remove redundant mid-execution auth gate from BlueprintSubcommand that caused
  duplicate [PASS] Azure Authentication output
- Fix RequirementCheck base class: use LogInformation for all check result lines
  to avoid WARNING:/ERROR: prefix doubling from logger formatter
- Collapse verbose requirements summary to single line:
  "Requirements: X passed, Y warnings, Z failed"
- Update tests to match new message text and log level assertions

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 7, 2026 02:37
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 38 out of 38 changed files in this pull request and generated 7 comments.


You can also share your feedback on Copilot code review. Take the survey.

sellakumaran and others added 4 commits March 8, 2026 01:14
Extends fail-early validation to setup infrastructure, setup permissions,
setup copilot-studio, cleanup azure, deploy, and publish commands.
Each command now runs targeted IRequirementCheck-based pre-flight checks
with formatted [PASS]/[FAIL] output before executing destructive or
slow operations, surfacing auth and config failures immediately.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…vocation UX

Phase 1 (zero behavioral change):
- Add GetBaseChecks() to SetupCommand and CleanupCommand for explicit check composition
- Add GetChecks() to each setup subcommand so check lists are co-located with their command
- Add RunChecksOrExitAsync() helper to RequirementsSubcommand to eliminate four-line boilerplate
- Guard all requirement check calls with if (!dryRun) to avoid spurious network calls
- Update RequirementsSubcommandTests to use public API after making internal helpers private

Fix CAE token revocation UX:
- Add ClientAppValidationException.TokenRevoked() factory for clear re-auth guidance
- Detect server-side CAE token revocation in GetClientAppInfoAsync and throw TokenRevoked
  instead of returning null (which was misreported as "app not found")
- Pass suppressErrorLogging: true to all az CLI calls in ClientAppValidator so raw error
  output no longer leaks to console before the formatted [FAIL] message
- Update ClientAppValidatorTests mocks to match suppressErrorLogging parameter

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…esults

AzureAuthValidator: add suppressErrorLogging to az account show call to
prevent CommandExecutor from printing raw stderr before [FAIL] output.
Remove verbose LogError/LogInformation guidance blocks — the validator
returns bool only; issue/resolution messaging belongs in the check layer.

PowerShellModulesRequirementCheck: downgrade auto-install progress from
LogInformation/LogWarning to LogDebug so they don't print before [PASS].

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add `--dry-run` flag to `a365 cleanup azure`: previews resources that
  would be deleted without requiring Azure auth or making any changes
- Add `AppServiceAuthRequirementCheck`: validates App Service deployment
  token before `a365 deploy`, catching AADSTS50173 token revocation early
- Add `MosPrerequisitesRequirementCheck`: validates MOS service principals
  before `a365 publish` proceeds, converting SetupValidationException to
  structured failure output
- Wire new checks into DeployCommand and PublishCommand via
  RunChecksOrExitAsync, replacing ad-hoc inline validation
- Add `GetChecks(AzureAuthValidator)` to InfrastructureSubcommand for
  explicit check composition
- Add `GetAppServiceTokenAsync` to AzureAuthValidator
- Update CLI design.md: add Requirements/ to project structure and
  document the IRequirementCheck prerequisite validation pattern
- Update CHANGELOG.md with user-visible additions

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 50 out of 50 changed files in this pull request and generated 8 comments.


You can also share your feedback on Copilot code review. Take the survey.

…aph, tests)

Exit codes (#7, #8/#9):
- Set Environment.ExitCode = 1 in ValidateDeploymentPrerequisitesAsync before
  each null return so callers exit non-zero on config/Web App validation failure
- Replace deploy-mcp guard `return` with ExceptionHandler.ExitWithCleanup(1)
  for AgentBlueprintId, AgenticAppId, and TenantId missing-config cases

Log severity (#15, #16, #17):
- LogCheckWarning: LogInformation -> LogWarning
- LogCheckFailure: all three LogInformation -> LogError
- ExecuteCheckWithLoggingAsync warning path: log ErrorMessage ?? Details
  so the primary warning message is no longer silently dropped

skip-graph regressions (#21, #22):
- Guard RunChecksOrExitAsync(MOS checks) behind if (!skipGraph)
- Guard clientAppId null check behind !skipGraph in PublishCommand

Unused parameter (#14):
- Remove IPrerequisiteRunner from BlueprintSubcommand.CreateCommand signature
- Update SetupCommand.cs call site and BlueprintSubcommandTests accordingly

InfrastructureRequirementCheck (#5, #6):
- Add I1/I2/I3/I1V2/I2V2/I3V2 (Isolated) SKUs to validation error message
- Wrap CheckAsync with ExecuteCheckWithLoggingAsync so [PASS]/[FAIL] is printed

PrerequisiteRunner warning message (#3):
- Log ErrorMessage ?? Details, log even when both are empty

IsCaeError gap (#18):
- Add InvalidAuthenticationToken to IsCaeError in ClientAppValidator

Stale comment (#10):
- Update ValidateDeploymentPrerequisitesAsync doc to remove "environment"

Tests (#19, #20):
- Add AppServiceAuthRequirementCheckTests (success, failure, metadata, null guard)
- Add MosPrerequisitesRequirementCheckTests (exception->failure, metadata, null guards)
- Update FrontierPreviewRequirementCheckTests: [WARN] now at LogWarning not LogInformation

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@sellakumaran sellakumaran enabled auto-merge (squash) March 9, 2026 17:37
…aph, tests)

- Remove redundant failure/warning logging from PrerequisiteRunner.RunAsync:
  ExecuteCheckWithLoggingAsync already emits [PASS]/[FAIL]/[WARN] output;
  the runner was re-logging the same error message and resolution guidance,
  producing ~5 lines of output for a single failed check instead of 3.
  Runner now only tracks the boolean pass/fail result.

- Fix \n escape in AppServiceAuthRequirementCheck resolution message:
  Raw \n does not render in log output; replaced with a single readable sentence.

- Fix double-error when config file is not found:
  ConfigService.LoadAsync was logging "Static configuration file not found"
  before throwing ConfigFileNotFoundException. The global exception handler in
  Program.cs then displayed the same message again via ExceptionHandler.
  Removed the pre-throw LogError; the exception message surfaces once.

- Fix redundant "Configuration file not found:" prefix in CleanupCommand and
  CreateInstanceCommand catch blocks: ex.IssueDescription already contains the
  full message including the path, so the extra prefix was duplicating it.

- Update PrerequisiteRunnerTests: RunAsync_WithWarningCheck_ShouldReturnTrue
  no longer asserts on warning logging from the runner (that is the check's
  responsibility via ExecuteCheckWithLoggingAsync).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 53 out of 53 changed files in this pull request and generated 5 comments.


You can also share your feedback on Copilot code review. Take the survey.

ajmfehr
ajmfehr previously approved these changes Mar 9, 2026
gwharris7
gwharris7 previously approved these changes Mar 9, 2026
- Remove AzureWebAppCreator, IPrerequisiteRunner, IAzureEnvironmentValidator
  from all command signatures (SetupCommand, AllSubcommand, BlueprintSubcommand,
  InfrastructureSubcommand) per PR #314
- Update Program.cs and test files to match new signatures
- Retain PR #312 fixes: remove double logging in PrerequisiteRunner,
  fix \n in AppServiceAuthRequirementCheck resolution message,
  suppress pre-throw logging in ConfigService

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@sellakumaran sellakumaran dismissed stale reviews from gwharris7 and ajmfehr via e61b3b8 March 9, 2026 20:55
gwharris7
gwharris7 previously approved these changes Mar 9, 2026
- DeployCommand: stop re-wrapping DeployAppException (was causing 3x stderr repetition)
- DeploymentService: throw short actionable exceptions instead of embedding full az cli stderr;
  move timing message after successful upload; remove duplicate [6/7] step label;
  pass suppressErrorLogging:true to ExecuteWithStreamingAsync
- CommandExecutor: add suppressErrorLogging param to ExecuteWithStreamingAsync;
  filter empty lines from stderr stream to avoid blank [Azure] prefixed lines
- NodeBuilder: downgrade CleanAsync log to Debug to remove out-of-sequence noise
- GraphApiService/FederatedCredentialService: extract clean error message from Graph JSON
  response body; move raw response body to LogDebug
- BlueprintSubcommand: replace multi-line FIC failure errors with single warning
- CleanConsoleFormatter: remove WARNING: text prefix from warning lines (keep yellow color)
- Tests: add regression tests for exception re-wrapping, site-start timeout, and generic az cli failure;
  update ExecuteWithStreamingAsync mocks for new suppressErrorLogging parameter;
  update CleanConsoleFormatter tests for new warning format

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 9, 2026 22:58
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 62 out of 62 changed files in this pull request and generated 3 comments.


You can also share your feedback on Copilot code review. Take the survey.

… cleanup)

- RequirementCheck: log both ErrorMessage and Details in warning path so
  FrontierPreview URL is no longer silently dropped
- DeploymentService: also match correct spelling "worker process failed to start"
  alongside the existing az cli typo to make timeout detection robust
- PrerequisiteRunner: update XML summary to accurately describe behavior
  (checks handle their own logging; runner only aggregates pass/fail)
- Program.cs: remove unused IPrerequisiteRunner/PrerequisiteRunner DI registration
  (no command injects it; checks run via RequirementsSubcommand.RunChecksOrExitAsync)
- design.md: fix Core Types snippet to match actual RequirementCheckResult API
  (Passed/IsWarning/ErrorMessage/ResolutionGuidance/Details, not RequirementCheckStatus/Issue/Resolution)
- design.md: fix FrontierPreviewRequirementCheck category from "Azure" to "Tenant Enrollment"
- MosPrerequisitesRequirementCheckTests: rewrite misleading test to actually exercise
  the MitigationSteps mapping branch by throwing SetupValidationException with known steps

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@sellakumaran sellakumaran merged commit 4d0929f into main Mar 10, 2026
8 checks passed
@sellakumaran sellakumaran deleted the users/sellak/failEarly branch March 10, 2026 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

All commands should validate and fail early if needed

4 participants